Codecov Report

Merging #4426 (eca634d) into master (e53605d) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4426      +/-   ##
==========================================
+ Coverage   55.11%   55.20%   +0.09%     
==========================================
  Files         397      397              
  Lines       50073    50073              
==========================================
+ Hits        27598    27645      +47     
+ Misses      20177    20142      -35     
+ Partials     2298     2286      -12     

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Verified:
The old flow: amd64 build (8m) + 4 downstream tasks (72m total, 27m max) => 27+8 = 25m run time
The new flow: 4 downstream tasks (68m total, 21m max) => 21m run time

Looks good but for whatever reason all downstream tasks are faster in the new configuration than in the old although they make more work. So questionable. Shouldn't we run the new more times to collect more data ensure the deviation from 21m we see is low?

the problem with this approach is that we are getting away from "build once, test/release many times". Now we will be building many times (which is not ideal) but ok. I'd rather look at what we are persisting and minimize that to what we actually need to persist.

for whatever reason all downstream tasks are faster in the new configuration than in the old although they make more work.

@algorandskiy Your observation is accurate though I think it can be explained. Here's a terse note, let me know if you'd like to discuss live.

• Here's the old arm64_test_verification: https://app.circleci.com/pipelines/github/algorand/go-algorand/8857/workflows/4c5b2c71-e077-41e8-89b1-e31cc1f2b032/jobs/159863. It spends 46s/60s or ~76% of its duration attaching to workspace.
• The new analog spends 0s on the same step: https://app.circleci.com/pipelines/github/algorand/go-algorand/8861/workflows/b221d409-e3f8-4ec6-a9b2-9baba4f1a70a/jobs/159909.
• Additionally, since the Go build process is ~1.5min with resource class = large and assuming ~45s to attach to work space in old topology, there's only a net ~45s overhead for moving the build downstream.

Shouldn't we run the new more times to collect more data ensure the deviation from 21m we see is low?

@algorandskiy A few thoughts:

• I definitely expect variability. Though I still expect the PR to be a net win.
• I kicked off several rebuilds: https://app.circleci.com/pipelines/github/algorand/go-algorand?branch=circleci_consolidate. Since the rebuilds are running in parallel, we may observe queuing due to CircleCI constraints. Which is to say, these runs may approximate a worst case scenario.

I'd rather look at what we are persisting and minimize that to what we actually need to persist.

@algojack I think you're asking a question that the group needs to fundamentally weigh. A few thoughts from my perspective:

• Are you suggesting to hold the PR until such an investigation happens? If so, let's speak live. I prefer to not hold the PR. I am open to being convinced to work on the problem after merging the PR.
• My somewhat loosely held opinion: Since the Go build completes quickly (< 3m) without vertically scaling up (resource class = medium), I think there's limited benefit to keeping the as-is topology.
  • Put another way, persisting to + reading from workspace must be appreciably < Go build duration for the as-is topology to be a net win.
  • Secondarily - Keeping the as-is topology implies cache maintenance. It's more friction to need to be aware of what's cached when, where, and why.
• Obviously, at some point, the assumption may no longer hold. Though my feeling is that the assumption will hold for the foreseeable future.

the problem with this approach is that we are getting away from "build once, test/release many times".

I think it's a mistake to think of CI as "release". It's just testing and the things that matter are speed, cost, and maintainability.

I think this PR improves the situation in all three dimensions.

It is demonstrably faster and cheaper, and is fewer lines and complexity in the .yml. The complexity of building in separate jobs, tied together by persisting specific items between the jobs was something we put up with because of the presumed advantages (in speed/cost) not something that was good for its own sake.

Looks good but for whatever reason all downstream tasks are faster in the new configuration than in the old although they make more work. So questionable. Shouldn't we run the new more times to collect more data ensure the deviation from 21m we see is low?

That's actually why we should do it. The downstream jobs were wasting time re-attaching a huge workspace created by the build, even though they didn't need. (Especially, for example, the test verification jobs that take less than a second, but were spending a minute re-attaching the workspace)

This time the slowest amd64 is again 20m so I guess I'm convinced.

the problem with this approach is that we are getting away from "build once, test/release many times".

I think it's a mistake to think of CI as "release". It's just testing and the things that matter are speed, cost, and maintainability.

We want to have reproducible build and I believe "build once test many times" also. So to consider this as being the build we compare our releases to, is not a mistake in my opinion.

not against this PR. Just wanted to discuss so that we are all aware of where we are headed with this. And we did discuss this on slack, so everyone seems to be ok with it.